Fix misleading notification when admin self-delete is rejected#1357
Open
MatusBeke wants to merge 1 commit into
Open
Fix misleading notification when admin self-delete is rejected#1357MatusBeke wants to merge 1 commit into
MatusBeke wants to merge 1 commit into
Conversation
…or text isSelfDeletionError() matches the backend's rejection message as plain text, but Spring Boot omits exception messages from error response bodies by default and DSpaceBadRequestException/IllegalStateException have no dedicated JSON-body exception handler, so the match can silently fail and fall through to the generic, unfriendly failure notification instead of the "you cannot delete your own account" one. Add a deterministic client-side identity check as a fallback alongside the text match so the friendly message shows reliably regardless of what the backend's error body contains. Fixes dataquest-dev/dspace-customers#782 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughDelete-failure handling in EPeopleRegistryComponent and EpersonFormComponent now shows the self-delete notification when the target equals the current user, in addition to the prior error-message-based check. Guard documentation was updated to reflect this fallback rationale, and new spec tests cover late-resolved user id scenarios. ChangesSelf-delete notification fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/access-control/epeople-registry/eperson-delete-guard.service.ts (1)
89-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider centralizing the self-delete OR-condition in the guard.
isCurrentUser(...) || isSelfDeletionError(...)is duplicated identically in bothEPeopleRegistryComponentandEPersonFormComponent. Adding a single combined method here would remove the duplication and prevent the two call sites from drifting apart in a future change.♻️ Proposed centralized helper
isSelfDeletionError(restResponse: RemoteData<NoContent> | null): boolean { return restResponse?.statusCode === 400 && restResponse?.errorMessage?.toLowerCase().includes('cannot delete yourself'); } + /** + * Whether the friendly self-delete notification should be shown for this delete attempt, + * combining the client-side identity check with the best-effort backend error-message check. + */ + shouldShowSelfDeleteNotification(ePerson: EPerson, currentAuthenticatedUserId: string, restResponse: RemoteData<NoContent> | null): boolean { + return this.isCurrentUser(ePerson, currentAuthenticatedUserId) || this.isSelfDeletionError(restResponse); + } +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/access-control/epeople-registry/eperson-delete-guard.service.ts` around lines 89 - 91, The self-delete check is duplicated as isCurrentUser(...) || isSelfDeletionError(...) in both EPeopleRegistryComponent and EPersonFormComponent. Add a single combined helper in EpersonDeleteGuardService, рядом with isSelfDeletionError, that encapsulates both conditions and use that method from both call sites to keep the logic centralized and prevent drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/access-control/epeople-registry/eperson-delete-guard.service.ts`:
- Around line 89-91: The self-delete check is duplicated as isCurrentUser(...)
|| isSelfDeletionError(...) in both EPeopleRegistryComponent and
EPersonFormComponent. Add a single combined helper in EpersonDeleteGuardService,
рядом with isSelfDeletionError, that encapsulates both conditions and use that
method from both call sites to keep the logic centralized and prevent drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0df45b1-046e-4099-badf-a6993f6db406
📒 Files selected for processing (5)
src/app/access-control/epeople-registry/epeople-registry.component.spec.tssrc/app/access-control/epeople-registry/epeople-registry.component.tssrc/app/access-control/epeople-registry/eperson-delete-guard.service.tssrc/app/access-control/epeople-registry/eperson-form/eperson-form.component.spec.tssrc/app/access-control/epeople-registry/eperson-form/eperson-form.component.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When an admin's own-account delete request is rejected by the backend, the frontend was supposed to show a friendly "you cannot delete your own account" message. It sometimes didn't, and fell through to the generic, technical failure notification instead.
Root cause
EPersonDeleteGuardService.isSelfDeletionError()detects the backend rejection by pattern-matching the response's error text against"cannot delete yourself". That text is not reliably present:messagefields from error response bodies by default (server.error.include-messagedefaults tonever).DSpaceBadRequestException/IllegalStateExceptionhave no dedicated@ExceptionHandlerthat builds a structured JSON body with the message.So a legitimate self-delete rejection can arrive with no usable message text, the pattern match silently fails, and the UI falls through to the generic "delete failed" notification instead of the friendly self-delete one.
Fix
Add a deterministic client-side identity check (
isCurrentUser) as an OR-fallback alongside the text match, in both delete entry points (EPeople registry list and the individual EPerson edit form). This doesn't depend on what the backend's error body contains — if we already know the target is the current user, we show the friendly message regardless.Test plan
yarn run lint— passesyarn run test:headless— full suite (5465 tests) passes, including 2 new regression tests simulating a race where the auth-subscription resolves late and the backend rejection carries no matchable messageyarn run build:prod— succeedsFixes dataquest-dev/dspace-customers#782
Summary by CodeRabbit